-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Configuration Method and expanded parameters for Database Model #14451
Conversation
cc6c632
to
0ee9d78
Compare
superset/models/core.py
Outdated
@@ -99,6 +100,11 @@ class CssTemplate(Model, AuditMixinNullable): | |||
css = Column(Text, default="") | |||
|
|||
|
|||
class ConfigurationMethod(str, enum.Enum): | |||
SQLALCHEMY_URI = "SQLALCHEMY_URI" | |||
DYNAMIC_FORM = "DYNAMIC_FORM" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit, but for consistency, we can make these values lowercase
superset/databases/schemas.py
Outdated
@@ -327,6 +329,7 @@ class Meta: # pylint: disable=too-few-public-methods | |||
description=cache_timeout_description, allow_none=True | |||
) | |||
expose_in_sqllab = fields.Boolean(description=expose_in_sqllab_description) | |||
configuration_method = EnumField(ConfigurationMethod, by_value=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a small description here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# configuration_method is used on the frontend to inform the backend whether to explode parameters or to provide only a sqlalchemy_uri
configuration_method = EnumField(ConfigurationMethod, by_value=True)
Does this look ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have two choices here.. let's discuss. When saving the database, the return is a sqlalchemy_uri, regardless of how the database was created. The new form is a two step form, so you basically create and then update. My current plan (pr isn't up yet, but I can paste it here for reference) is to not provide the configuration_method in the put. There are no plans as of now to allow people to switch from one to the other once they have created the database. But @yousoph and @Steejay and I have been talking about this, so it could change. The other option is to return the parameters and the configuration_method value with the save db response, which would be a more flexible solution, but might take more time to implement, and as it looks now we don't need it, so it could wait for a v2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AAfghahi you should add the description to the field, this way it shows up in the Swagger interface:
configuration_method = EnumField(ConfigurationMethod, by_value=True, description="...")
if database_name: | ||
# Check database_name uniqueness | ||
if not DatabaseDAO.validate_update_uniqueness( | ||
self._model_id, database_name | ||
): | ||
exceptions.append(DatabaseExistsValidationError()) | ||
if configuration_method: | ||
if ConfigurationMethod(configuration_method) not in { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this? I believe if the value is not in the enum you're going to get an error, and you'll have to catch the error instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_update_database_with_invalid_configuration_method(self):
"""
Database API: Test update
"""
example_db = get_example_database()
test_database = self.insert_database(
"test-database", example_db.sqlalchemy_uri_decrypted
)
self.login(username="admin")
database_data = {
"database_name": "test-database-updated",
"configuration_method": "BAD_FORM",
}
uri = f"api/v1/database/{test_database.id}"
rv = self.client.put(uri, json=database_data)
self.assertEqual(rv.status_code, 400)
THis is the test I wrote for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool. can you maybe assert the error message, too, to validate that you're getting the right error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @eschutho, the 400 you're seeing in your test is probably a generic error:
>>> from enum import Enum
>>> class Foo(Enum):
... A = "A"
...
>>> Foo("A")
<Foo.A: 'A'>
>>> Foo("B")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/beto/.pyenv/versions/3.9.2/lib/python3.9/enum.py", line 360, in __call__
return cls.__new__(cls, value)
File "/Users/beto/.pyenv/versions/3.9.2/lib/python3.9/enum.py", line 677, in __new__
raise ve_exc
ValueError: 'B' is not a valid Foo
You should do something like this instead:
if configuration_method not in {method.value for method in ConfigurationMethod}:
superset/models/core.py
Outdated
@@ -36,6 +36,7 @@ | |||
Column, | |||
create_engine, | |||
DateTime, | |||
Enum, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I forgot to remove this when I converted to a string.. @AAfghahi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was failing a lint test, so I removed this.
296dbce
to
5e0a541
Compare
ConfigurationMethod.SQLALCHEMY_URI, | ||
ConfigurationMethod.DYNAMIC_FORM, | ||
}: | ||
exceptions.append(DatabaseExistsValidationError()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use a different exception here, since DatabaseExistsValidationError
is used when a database that is being created already exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would DatabaseRequiredFieldValidationError
be better?
if database_name: | ||
# Check database_name uniqueness | ||
if not DatabaseDAO.validate_update_uniqueness( | ||
self._model_id, database_name | ||
): | ||
exceptions.append(DatabaseExistsValidationError()) | ||
if configuration_method: | ||
if ConfigurationMethod(configuration_method) not in { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @eschutho, the 400 you're seeing in your test is probably a generic error:
>>> from enum import Enum
>>> class Foo(Enum):
... A = "A"
...
>>> Foo("A")
<Foo.A: 'A'>
>>> Foo("B")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/beto/.pyenv/versions/3.9.2/lib/python3.9/enum.py", line 360, in __call__
return cls.__new__(cls, value)
File "/Users/beto/.pyenv/versions/3.9.2/lib/python3.9/enum.py", line 677, in __new__
raise ve_exc
ValueError: 'B' is not a valid Foo
You should do something like this instead:
if configuration_method not in {method.value for method in ConfigurationMethod}:
superset/databases/schemas.py
Outdated
@@ -327,6 +329,7 @@ class Meta: # pylint: disable=too-few-public-methods | |||
description=cache_timeout_description, allow_none=True | |||
) | |||
expose_in_sqllab = fields.Boolean(description=expose_in_sqllab_description) | |||
configuration_method = EnumField(ConfigurationMethod, by_value=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AAfghahi you should add the description to the field, this way it shows up in the Swagger interface:
configuration_method = EnumField(ConfigurationMethod, by_value=True, description="...")
2219035
to
140ef90
Compare
Codecov Report
@@ Coverage Diff @@
## master #14451 +/- ##
==========================================
+ Coverage 77.45% 77.54% +0.09%
==========================================
Files 958 958
Lines 48533 48541 +8
Branches 5702 5702
==========================================
+ Hits 37592 37642 +50
+ Misses 10740 10698 -42
Partials 201 201
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the extra import, this LGTM. thanks @AAfghahi!
superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py
Outdated
Show resolved
Hide resolved
7f483ec
to
213cb60
Compare
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
16a032d
to
95b51b3
Compare
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
95b51b3
to
3d05a71
Compare
This change has broken the superset-frontend though for master. configuration_method is not being passed from the database add modal. Works fine for edit since I'm guessing it's loaded from the api but when creating a new one it's giving an error like the attached. My guess is superset-frontend/src/views/CRUD/data/database/types.ts needs to be updated, but I'm not too great at React to be sure. |
@sujeetpillai I think this will be fixed with: #14583 Though for the time being I am pushing up this change to make it optional; #14668 |
@AAfghahi Thanks for the quick fix. I can confirm it's now fixed. |
Thank you so much, I can confirm it's fixed as well
…On Tue, May 18, 2021, 7:07 AM Sujeet Pillai ***@***.***> wrote:
@AAfghahi <https://github.com/AAfghahi> Thanks for the quick fix. I can
confirm it's now fixed.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#14451 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA3VSVXR7NJW2ZJBFXPHPSLTOHYXTANCNFSM44BTZ7MQ>
.
|
Nice! Glad to hear it. |
…apache#14451) * db migration for dbs * first draft at logic * added unit tests * revisions * use strings for db values * lint and revisions to tests * changed test back * added revisions for testing * Update superset/databases/commands/update.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/databases/schemas.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/models/core.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/databases/commands/update.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * got rid of extra imports added new test * Update superset/databases/schemas.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
…apache#14451) * db migration for dbs * first draft at logic * added unit tests * revisions * use strings for db values * lint and revisions to tests * changed test back * added revisions for testing * Update superset/databases/commands/update.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/databases/schemas.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/models/core.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/databases/commands/update.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * got rid of extra imports added new test * Update superset/databases/schemas.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
…apache#14451) * db migration for dbs * first draft at logic * added unit tests * revisions * use strings for db values * lint and revisions to tests * changed test back * added revisions for testing * Update superset/databases/commands/update.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/databases/schemas.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/models/core.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/databases/commands/update.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * got rid of extra imports added new test * Update superset/databases/schemas.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
SUMMARY
We are creating an alternative way of editing or creating a database, in order for this to be feasible we want to add a configuration_method column to the database model.
This PR adds the necessary logic to the model, with validation, and tests to make sure that it is working.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
Unit tests added.
ADDITIONAL INFORMATION